Repository navigation
Conversation
|
@uros-b PTAL when available. |
|
@uros-b friendly ping, PTAL when available. Thanks! |
nchammas
left a comment
There was a problem hiding this comment.
LGTM pending wording suggestion.
| "Cannot retrieve tables or views that do not belong to the same database. Requested: <qualifiedTableNames>." | ||
| ], | ||
| "sqlState" : "0A000" |
There was a problem hiding this comment.
Since the error state maps to "feature not supported", I would tweak the message to use that wording.
For example: "Retrieving tables or views from more than one database is not supported. Requested: <qualifiedTableNames>."
This also avoids the double negative of "cannot retrieve ... that do not belong".
There was a problem hiding this comment.
@nchammas ty, I have updated to avoid the double negative and to account for the sqlState change mentioned below.
| "message" : [ | ||
| "Cannot retrieve tables or views that do not belong to the same database. Requested: <qualifiedTableNames>." | ||
| ], | ||
| "sqlState" : "0A000" |
There was a problem hiding this comment.
Is 0A000 right here?
0A000 is "feature not supported". This is a same-database restriction on SessionCatalog.getTablesByName, not an unimplemented feature.
CANNOT_RENAME_ACROSS_SCHEMA already uses 0AKD0 ("Cross catalog or schema operation not supported"), which is a closer match. RENAME_TABLE_SOURCE_DESTINATION_DATABASE_MISMATCH uses 3F000. I'd prefer 0AKD0 here unless there is a reason to stay on 0A000.
There was a problem hiding this comment.
0AKD0 is Databricks-specific, no? It is a closer match, though.
There was a problem hiding this comment.
I believe 0AKD0 is specific to Databricks (link). I also couldn't find any reference to it in the ISO/IEC 9075 standard.
However, I noticed in the repo that 0AKD0 is used in error-conditions.json on line 899:
"CANNOT_RENAME_ACROSS_SCHEMA" : {
"message" : [
"Renaming a <type> across schemas is not allowed."
],
"sqlState" : "0AKD0"
},
I'm not sure whether this sets a precedent or whether this topic has been discussed before. I am OK with either option and can update the message and sqlState accordingly.
There was a problem hiding this comment.
Sure, Databricks originated the subclass, but this doesn't necessarily mean that OSS Spark cannot use it. For example, CANNOT_RENAME_ACROSS_SCHEMA already ships 0AKD0 in Spark.
I still prefer 0AKD0 here. 0A000 is the generic "feature not supported" bucket. This is a cross-database restriction on SessionCatalog.getTablesByName, the same shape as a cross-schema rename.
@srielau WDYT? which state for this catalog restriction
There was a problem hiding this comment.
Correct, from day one we have been using SQLSTATEs outside those listed in the SQL standard, which have been popularized by other vendors. The goal of 0AK vs 0AKD, was to minimize conflicts. Once a SQLSTATE has been assigned, everything speaks for reusing it, as long as we are faithful in its meaning.
That being said, we must remember that the SQL Standard does NOT Have error conditions or SQLCODEs. So our need to differentiate between different kinds of 0A*** is largely mitigated by being able to go all in with error condition names.
As such, I would not consider the debate of 0A000 vs 0AKD0 vs 0AK00 as fundamenmtal.
The finer the SQLSTATE, the easier it is to collect metrics on a set of condition.
There was a problem hiding this comment.
Yes, the 0A error class is appropriate, and the sub-class is not as important to get exactly right. I think 0AKD0 is a good choice, too.
the SQL Standard does NOT Have error conditions or SQLCODEs
Side point but: Did you mean to say SQLSTATE instead of SQLCODE here?
SQLCODE is a deprecated part of the SQL standard. SQLSTATE is a different thing (which maps to our error states), and the standard very much does have SQLSTATEs (search for "Table_23").
There was a problem hiding this comment.
"SQL Standard does NOT Have error conditions or SQLCODEs" I think we agree.
All the SQL Standard has are SQLSTATEs
…OT_IN_SAME_DATABASE
- Avoided the double negative in the message.
- Change the SQLSTATE from 0A000 to 0AKD0 ("Cross catalog or schema
operation not supported")
…y-error-1072 # Conflicts: # common/utils/src/main/resources/error/error-conditions.json
f23b7fe to
fe89468
Compare
What changes were proposed in this pull request?
Rename the legacy error condition
_LEGACY_ERROR_TEMP_1072toTABLES_OR_VIEWS_NOT_IN_SAME_DATABASE(SQLSTATE0AKD0). It is raised bySessionCatalog.getTablesByNamewhen the requested tables/views span more than one database. The message is also reworded for clarity.Why are the changes needed?
The error-conditions README disallows new
_LEGACY_ERROR_TEMP_*entries and asks existing ones to be resolved. This resolves one (part of SPARK-37935).Does this PR introduce any user-facing change?
Yes. The condition becomes
TABLES_OR_VIEWS_NOT_IN_SAME_DATABASEwith SQLSTATE0AKD0("Cross catalog or schema operation not supported"), and the message is reworded to "Retrieving tables or views from more than one database is not supported. Requested: ." SQLSTATE0AKD0matches the cross-schema restriction and follows the precedent ofCANNOT_RENAME_ACROSS_SCHEMA, rather than the more generic0A000("feature not supported").How was this patch tested?
Strengthened the existing
SessionCatalogSuitetest to assert the condition and parameters viacheckError(it previously only intercepted the exception).SparkThrowableSuitepasses.Was this patch authored or co-authored using generative AI tooling?
Co-authored with Claude Opus 4.8